feat(cli): add healthcheck, foreground supervision, and configurable cert SANs - #230
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
4b7c4c1 to
7aa3895
Compare
|
Thanks @robinnsc, this is a well-put-together PR and the description matches what the code does. I built the branch and ran everything live against a real PostgreSQL before writing this, so the findings below are observed, not read. What I verifiedAll claims in the description hold on this machine (release build, real Postgres):
One functional issue worth fixing before mergeWildcard SANs break the idempotent entrypoint. The generation path ( Smaller observations (non-blocking)
The security-adjacent choices are right: skipping cert verification in the probe is correct for a self-signed liveness check and is honestly documented, and using rustls' real name verification for SAN coverage (rather than string matching) handles the IP-vs-DNS distinction properly. Sequencing and the containerization planTwo coordination points, neither a fault of this PR:
Net: fix the wildcard SAN check, rebase once #218 lands, and this is good to go from my side. |
7aa3895 to
10bb920
Compare
|
Rebased onto main and addressed those callouts:
Will address those doc gap callouts on those various design docs |
|
Really nice PR. I verified everything in the description live and it all holds, including the try-every-resolved-address handling, which quietly saves an IPv6-only deployment from a wrong verdict. Gates are clean (fmt, clippy One thing I would like fixed, then a few take-them-or-leave-them notes. Please fix:
|
10bb920 to
a4784df
Compare
a4784df to
36544ce
Compare
|
Thanks, addressed those comment. The probe now creates one three second deadline up front. DNS resolution, all resolved-address connection attempts, the rustls handshake, request writes, and response reads consume that same budget. The transport wrapper recalculates the remaining time before every underlying socket read or write, including the calls rustls makes internally, so partial progress cannot reset the clock. DNS runs behind the same deadline as well, rather than leaving synchronous resolution as an unbounded step. I also replaced the unbounded read_to_string with an 8 KiB capped byte read. Only the first HTTP status line is parsed, so reading beyond that prefix cannot improve the verdict. There are two regression tests. The first completes TLS, receives and validates GET /health, sends HTTP/1.1 200 OK, then emits one byte per second. The old implementation remained alive until the ten-second test harness killed it; the fixed probe exits non-zero at the shared deadline, after the test has observed both the status line and partial read progress. The second sends more than 16 KiB and deliberately keeps the connection open: the fixed probe returns successfully after the 8 KiB cap instead of waiting for EOF. The TLS test helper also propagates worker-thread failures and has bounded cleanup so either test cannot pass on an unrelated prompt failure. |
…cert SANs Make the binary supervisable and probeable from a container runtime. First of two changes for container readiness; the migration concurrency guard follows separately. - healthcheck: new subcommand that probes /health over HTTPS and exits 0 or 1, so a Docker HEALTHCHECK needs no shell or curl and works on distroless. It reports liveness, which is what a HEALTHCHECK and a Kubernetes livenessProbe want: /health is a static handler that does not query the backend, and a liveness probe that failed on a database outage would restart every replica at once. There is no readiness endpoint yet; adding one backed by a cached storage-layer round-trip is the follow-up. Address resolution, connect, TLS handshake, request write, and response read share one 3s deadline. The remaining budget is applied before every underlying socket call, so partial progress cannot reset the timeout, and the response is capped at 8 KiB since only its status line is used. Every resolved address is tried so a name resolving to both ::1 and 127.0.0.1 works, and --endpoint accepts an optional scheme, port, path, and IPv6 literal. The flagless probe derives its host from the configured bind_addr rather than assuming 127.0.0.1, so an IPv6-bound server is not reported unhealthy, and --port mirrors serve's own override. Timeout-configuration failures are propagated, since a bounded probe is the whole point of the command. - serve --foreground: write no PID file and skip the run directory by default, so the container can use a read-only root filesystem. Daemon mode is unchanged. With no PID file to read, `stop` now probes the port and reports that a server is listening under foreign supervision rather than claiming nothing is running; `status` already degrades to an unknown PID. - serve --write-pid-file: opt back into the PID file in foreground mode, for shell use and tooling that wants `stop` and `status` to work. It goes to the same run_dir path daemon mode uses, so neither command needs extra arguments, and run_dir then has to be writable. Ignored in daemon mode, which always writes one. devtools/run-tests restarts the server with `stop` to apply a config change, so the integration workflow passes this flag; without it that restart silently did nothing: `stop` failed, `serve` could not bind, and the health check passed against the process that was never replaced. That path no longer suppresses errors either, so a failed restart fails the run instead of reporting success. - init --tls-san <name> (repeatable): append Subject Alternative Names to the generated self-signed certificate so it is valid for the name clients use, such as an in-cluster service DNS name, not just localhost/127.0.0.1/bind-addr. Values are trimmed and de-duplicated case-insensitively. init never regenerates an existing certificate, so a later --tls-san cannot take effect; rather than exit 0 having dropped the name and leave clients to hit a TLS hostname verification failure, it verifies the existing certificate covers every requested SAN and fails with an actionable error otherwise. Certificate generation moved ahead of all database work so a bad SAN fails before any state is created. The coverage check uses rustls's own `verify_server_name`, so it adds no new dependency. A wildcard such as *.svc.cluster.local is a valid certificate entry but not a valid server name, so coverage is tested by substituting a single label; without that, a wildcard accepted on the first run failed on every later one, which is a crash loop for the idempotent entrypoint. Every requested name is validated before generation, so a malformed wildcard fails on the first run rather than the next. - docs: correct the architecture and deployment guides, which claimed the server always daemonizes and that foreground mode still writes a PID file, and replace the container recipe that waited on that PID file with a foreground entrypoint plus a HEALTHCHECK. - tests: add tests/test_cli_container_readiness.py covering SAN generation, dedup/blank handling, the not-covered failure and the already-covered idempotent case, healthcheck up/down/--endpoint, prompt failure against an unreachable host, a TLS peer that drips one byte per second, an oversized response held open past the 8 KiB cap, and foreground leaving no PID file or run directory while still exiting on SIGTERM. Runs under an isolated $HOME so the suite no longer overwrites the developer's real ~/.extenddb certificate. - devtools/run-tests: exclude the new file from the main pytest suite and run it in the CLI section instead, alongside test_cli_lifecycle.py. Like those tests it starts and stops its own servers and creates its own databases, so it cannot run in parallel against the shared instance the main suite uses. Rebased onto the post-#218 layout: the CLI now lives in crates/app, so cmd_healthcheck joins it there. ServeParams gains pid_file: Option<PathBuf> in place of run_dir, which it only ever used to derive that path, so serve() no longer writes a PID file unconditionally and needs no notion of a run directory.
36544ce to
fc5f80a
Compare
3 participants
What
Makes the
extenddbbinary supervisable and probeable from a container runtime. First of two container-readiness PRs; the migration concurrency guard follows in a separate PR.extenddb healthcheck— new subcommand that sends an HTTPSGET /healthand exits 0 or 1, so a DockerHEALTHCHECKneeds no shell orcurland works on adistroless/scratchbase. Reads the port from the config file, or takes--endpoint https://host:port. Connect, read, and write are bounded at 3sTcpStream::connecthas no timeout of its own), every resolved address is tried so a name resolving to both::1and127.0.0.1works, and--endpointaccepts an optional scheme, port, path, and IPv6 literal.serve --foregroundwrites no PID file and skips the run directory entirely, so the container can use a read-only root filesystem. Daemon mode is unchanged. With no PID file to read,stopnow probes the port and reports that a server is listening under foreign supervision instead of claiming nothing is running;statusalready degraded to reporting an unknown PID.serve --write-pid-file— opts back into the PID file in foreground mode, for shell use and for tooling that wantsstopandstatusto work. It goes to the samerun_dirpath daemon mode uses, so neither command needs extra arguments, andrun_dirthen has to be writable. Ignored in daemon mode, which always writes one.init --tls-san <name>(repeatable) — appends Subject Alternative Names to the generated self-signed certificate so it is valid for the name clients actually use, such as an in-cluster service DNS name, rather than onlylocalhost/127.0.0.1/bind-addr. Values are trimmed and de-duplicated case-insensitively.HEALTHCHECK.devtools/run-tests' CLI section instead, alongsidetest_cli_lifecycle.py; like those tests it starts and stops its own servers and creates its own databases, so it cannot run in parallel against the shared instance the main suite uses. The integration workflow now starts the server with--write-pid-file, becauserun-testsrestarts it viaextenddb stopto apply an import/export config change. That restart also no longer suppresses errors — see Notable below.Implementation Decisions
healthcheckis a liveness probe, deliberately./healthis a static handler that does not query the storage backend, so a replica whose database has gone away still reports healthy. That is the right behaviour for aHEALTHCHECKand a KuberneteslivenessProbe: one that failed on a database outage would restart every replica at once and prolong the outage. A backend that is unreachable at startup does stop the server from listening, so that case is caught. There is no readiness endpoint yet, and the follow-up is to add one backed by a cached storage-layer round-trip rather than to make/healthquery the backend and lose its value as a liveness signal. This is stated in the module docs, the admin guide, and the deployment guide so nobody wires it to areadinessProbeexpecting traffic to drain.initfails rather than silently dropping a--tls-san.initnever regenerates an existing certificate, since rotating the key pair under a live deployment would be a surprise. That means a--tls-sanadded on a later run cannot take effect — and the container story generates the certificate into a persistent volume with an idempotent entrypoint that re-runsiniton every start, so this is the common path, not an edge case. Exiting 0 having dropped the name leaves the operator to discover it as a client-side TLS hostname verification failure. Instead, when a certificate already exists we verify it covers every requested SAN and fail with an actionable error otherwise; an already-covered SAN is accepted, so the idempotent entrypoint still works. Certificate generation also moved ahead of all database work so a bad SAN fails before any users or databases are created.Why
Prerequisite binary changes from the containerization design: the server daemonizes by default (so the container runtime sees PID 1 exit), needs a health probe that works without a shell, and fixes its certificate SANs to localhost/bind-addr, which is wrong for any in-cluster service name.
Testing done
New
tests/test_cli_container_readiness.py, against a real PostgreSQL:--tls-sanadds one and multiple SANs to the generated certificate; blanks are skipped and case-insensitive duplicates appear once.initfails, naming the SAN, when an existing certificate does not cover it, and does not rotate the certificate; an already-covered SAN is accepted.healthcheckexits 0 when the server is up, non-zero before start and afterstop, honours--endpointincluding a trailing path, and fails in under 15s against an unreachable host instead of hanging for the OS connect timeout.serve --foregroundleaves no PID file and no run directory, answershealthcheck, is not killed byextenddb stop(which reports the port is listening), and exits on SIGTERM.Plus 5 unit tests for
--endpointparsing (scheme, path, default port, IPv6, malformed input).Also verified manually that the tests fail if the SAN coverage check is removed, so they are not passing by accident, and that the suite no longer touches the real
~/.extenddb/tlsChecklist
cargo test --workspace)cargo fmt --check)cargo clippy -- -W clippy::pedantic)Storagetrait, auth model, on-diskformat, or public CLI surface, an RFC has been accepted or is linked
below. Otherwise, an ADR captures the decision (link below).
Breaking changes
serve --foregroundno longer writes a PID file or createsrun_dir. Onmainit does both, andextenddb stopworks against a foreground server. Anyone relying on that must add--write-pid-file, which restores the previous behaviour exactly.extenddb statusis unaffected apart from reporting the PID as unknown, since it probes the port. Daemon mode is unchanged.The repo's own tooling was such a consumer:
devtools/run-testsrestarts the server withextenddb stop, so the integration workflow passes the new flag.One newly non-silent failure:
init --tls-san Xagainst an existing certificate that does not coverXnow exits non-zero where it previously exited 0 and ignored the flag. Since--tls-sanis new in this PR, no existing invocation can hit it.By submitting this pull request, I confirm that my contribution is made under
the terms of the Apache License 2.0 and I agree to the Developer Certificate of
Origin (DCO). See CONTRIBUTING.md for details.